fix(plugins): Drop mutex between token-exchange and token-broker - #404
Merged
huang195 merged 1 commit intoMay 21, 2026
Merged
Conversation
PR rossoctl#400 migrated both plugins to declare `Claims: []string{contracts.ClaimAuthorizationHeader}`, which made plugins.Build reject any chain that contained both. On review, David pointed out this diagnosis was wrong: token-exchange and token-broker have legitimate coexistence use cases (the former handles Keycloak- audience hosts, the latter handles federated-broker hosts, same deployment may need both on different routes). The correct fix for their interaction is per-route arbitration — "which handler runs for this specific host" — not per-chain mutual exclusion. Route-scoped arbitration is a separate design (still open on rossoctl#398). This commit is the narrow revert that restores coexistence. Reverts: - tokenexchange/plugin.go: Capabilities() returns empty PluginCapabilities; contracts import dropped. - tokenbroker/plugin.go: same. - plugins_test.go: removes TestBuild_TokenExchangeAndTokenBroker_ConflictingClaims (the integration test that locked in the mutex) and the tokenbroker side-effect import that only existed for that test. Retained on purpose (no behavior change, no consumer today): - PluginCapabilities.Claims field + validateRelationships Claims loop + related tests in registry_test.go — the framework-level mutex mechanism is general-purpose and may have valid uses for plugins that genuinely conflict chain-wide rather than per- route. No in-tree plugin currently declares Claims. - authlib/contracts/claims.go + ClaimAuthorizationHeader constant — kept so that a future plugin declaring this specific claim doesn't have to re-invent the vocabulary. Documented as unreferenced-by-in-tree-plugins to avoid signal confusion. Operationally: after this lands, configuring both token-exchange and token-broker in the same outbound chain becomes the operator's responsibility to partition cleanly across routes — same as the pre-rossoctl#400 status quo. The plugins' own OnRequest behavior is unchanged by this revert (it was also unchanged by rossoctl#400 — the mutex was build-time only). Relates to rossoctl#398. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
huang195
force-pushed
the
fix/unmark-token-handlers-mutually-exclusive
branch
from
May 13, 2026 12:04
7d315e5 to
05e0fd0
Compare
Contributor
|
/lgtm |
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Rossoctl Issue Prioritization
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Narrow revert addressing @davidhadas's review feedback on #398 /
PR #400: token-exchange and token-broker should be allowed to coexist
in the same outbound chain. Their interaction needs per-route
arbitration ("which handler runs for this specific host?"), not
per-chain mutual exclusion.
PR #400 shipped the wrong diagnosis. It treated the two plugins'
overlap on the Authorization header as a chain-scoped claim
conflict and locked them out of coexistence at
plugins.Buildtime. As David points out on #398, a single deployment may
legitimately need both — token-exchange for Keycloak-audience
hosts, token-broker for federated-broker hosts, routed per-host.
What this PR does
Reverts the two-plugin mutex. Specifically:
tokenexchange/plugin.go—Capabilities()returns emptyPluginCapabilities;contractsimport dropped.tokenbroker/plugin.go— same.plugins/plugins_test.go— removesTestBuild_TokenExchangeAndTokenBroker_ConflictingClaims(theintegration test that locked in the mutex) and the tokenbroker
side-effect import that only existed for that test.
What this PR deliberately does NOT do
PluginCapabilities.Claimsfield or thevalidateRelationshipsClaims loop or its tests inregistry_test.go. The framework-level mutex mechanism isgeneral-purpose; a future plugin with a genuine chain-wide
conflict (not this route-scoped one) may declare it. No in-tree
plugin currently does.
contracts.ClaimAuthorizationHeader. Kept asa dormant constant so that a future plugin declaring this
specific claim doesn't have to re-invent the vocabulary.
design David proposed in feature: Core Arbitration for Overlapping Plugin Responsibilities in Kagenti AuthBridge #398 (responsibility strings + plugin-
ordered route precedence, with an explicit passthrough plugin as
the catch-all). It's a separate PR, pending design alignment on
the issue thread.
Behavior
After this lands:
plugins.Build.the two plugins (same as the pre-feat: Plugin relationship declarations + token-exchange/token-broker claim migration #400 status quo — this PR
restores that status quo).
OnRequest/ routing logic was changed byfeat: Plugin relationship declarations + token-exchange/token-broker claim migration #400 and neither is changed by this revert.
Test plan
go test -race ./...inauthbridge/authlib— passesgo test -race ./...inauthbridge/cmd/authbridge— passesgofmt -lclean on touched filesRelates to #398.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com